Feat: Add token-broker service for OAuth session and token management#449
Feat: Add token-broker service for OAuth session and token management#449davidhadas wants to merge 1 commit into
Conversation
0efc5f4 to
cd98d27
Compare
|
Thanks for the work on this, David. I don't want to block your research progress, but a few observations from the productization side might be still ok: Completeness: The token-broker cannot complete an OAuth flow end-to-end without the Backend service (Phase 3) that handles the user-facing browser interaction. JWT signature validation is also explicitly skipped. As it stands, this is a Phase 1 foundation, not a shippable component. Keycloak overlap: The core functionality (authorization code + PKCE, token caching, JWT expiry parsing, session management) overlaps significantly with what Keycloak provides natively through identity brokering. The implementation also reimplements standard Go library functionality ( Repo integration: The token-broker is architecturally independent (separate binary, separate Deployment, zero imports from operator code), but it shares the operator's For a long term solution, maybe consider extracting the token-broker into its own module/repo and evaluating whether Keycloak identity brokering or CIBA could address the external-provider token acquisition gap before building a custom service. Happy to discuss the Keycloak/CIBA angle further (see also the discussion on #1435). |
cd98d27 to
0246162
Compare
|
Thanks for the review.
Agreed. We are laying down the HITL foundation. More work is definitely needed, but we are at a phase where we can finally see the landscape and discuss the design decisions, controls, configs, and user experience. Does this mean we should hold off on merging this PR once it passes review? Or can we merge it post-review with the explicit note that we still have work before it is production-ready? Repo integration: Understood. So, keep it in this repo but ensure it uses its own isolated go.mod. Relationship with Keycloak: Let’s keep that specific discussion over to the Epic issue kagenti/kagenti#1435 as it is part of the big picture we need to discuss. It should be noted here though that token broker service should have a later PR to include integration with KeyCloak supporting use cases in which Kagneti is deployed with KeyCloak and need to offer OAuth 2 against federated systems. |
8495d4a to
1e49387
Compare
|
Thanks David. To be clear: please don't feel blocked by me on merging this. My comments were architectural observations, not merge blockers. I've come to better appreciate that kagenti's upstream is designed for research incubation and rapid iteration, without stability guarantees that would slow contributors down. That's a valid and valuable mode of operation. The productization constraints I've been raising come from a different set of goals, and it's not fair to apply those as a gate on upstream research work. Go ahead and merge when it passes review. The foundation you're laying here is solid exploration of the HITL problem space. Separate |
b942b2b to
88af9aa
Compare
|
@rhuss , |
The token-broker service provides centralized OAuth token acquisition for AI agents via PKCE-based authorization flows, token caching with JWT expiry parsing, and session management with long-polling coordination. Signed-off-by: David Hadas <david.hadas@gmail.com>
88af9aa to
46cff1b
Compare
|
|
||
| // CleanupExpiredTokens removes all expired tokens from the cache. | ||
| // This can be called periodically to free memory. | ||
| func (tc *TokenCache) CleanupExpiredTokens() int { |
There was a problem hiding this comment.
CleanupExpiredTokens() is defined but never wired to a ticker in main.go. For long-running instances, expired tokens will accumulate in memory. Suggestion: add a periodic cleanup
goroutine, or clean up lazily on SetToken when the session map exceeds a threshold.
|
|
||
| // OAuthDiscoverer performs OAuth discovery against a resource server. | ||
| // Updated to support the new architecture where Token Broker is the OAuth client. | ||
| type OAuthDiscoverer interface { |
There was a problem hiding this comment.
the OAuthDiscoverer interface is tightly coupled to the OAuth authorization code + PKCE flow.
If we want to support alternative token acquisition mechanisms (e.g., Keycloak identity brokering, CIBA, or custom IdP flows), it might be worth defining a higher-level TokenAcquirer
interface that abstracts the grant type entirely, with PKCE as one implementation.
| } | ||
|
|
||
| // writeBackendError writes a standardized error response (nested format for Backend API). | ||
| func writeBackendError(w http.ResponseWriter, status int, code, message string) { |
There was a problem hiding this comment.
The API uses two different error response formats: writeError (flat, for AuthBridge) and writeBackendError (nested, for Backend). This makes client integration harder — consumers need
to know which endpoint returns which format. Consider standardizing on a single error envelope.
| labels: | ||
| app: token-broker | ||
| spec: | ||
| replicas: 1 # In-memory sessions/token cache: do NOT scale >1 without shared state. |
There was a problem hiding this comment.
Already discussed in the Slack thread — the replicas: 1 constraint with in-memory state is a known limitation. Just noting that the security posture is good (non-root, read-only
filesystem, capabilities dropped).
Keycloak overlap questionRoland's observation about the overlap with Keycloak identity brokering is worth exploring further. The core of what the token-broker implements — authorization code + PKCE, token caching, JWT expiry parsing, session management — maps closely to what Keycloak provides natively. CIBA (Client-Initiated Backchannel Authentication) is also an interesting alternative for the HITL use case: the backend initiates the auth request, the user consents via a separate channel, no browser redirect needed. Provider-agnostic token brokering interfaceThat said, tying the HITL flow exclusively to Keycloak would create a hard dependency on a specific IdP. Since we're actively working on decoupling from Keycloak on the AuthBridge side, it raises a broader design question: could we define a provider-agnostic interface for token brokering, where Keycloak is one possible backend but not the only one? Concretely, the current |
|
@akram - all good points. Thank you for the review. I will address one by one and present for additional review. Let record the overall solution directions discussions in the epic kagenti/kagenti#1435 - I think some of your comments related to Keycloak overlap question were already discussed there and maybe you can comment when it was not. Related to Provider-agnostic token brokering interface, I would like to consider the proposed higher-level TokenAcquirer abstraction and come back with thoughts. |
Summary
The token-broker service provides centralized OAuth token acquisition for AI agents via PKCE-based authorization flows, token caching with JWT expiry parsing, and session management with long-polling coordination.
Fixes: #455
Related issue(s)
kagenti/kagenti#1435